feat(wallet-sdk): accounts slice (step 6) by pmilic021 · Pull Request #1167 · MakePrisms/agicash

pmilic021 · 2026-07-13T09:46:33Z

Step 6 of the wallet-SDK migration (docs/superpowers/specs/2026-06-24-wallet-sdk-no-cache-production-design.md): wrap the accounts domain in AccountsApi and flip the web's accounts data layer onto sdk.accounts.*. Plan with the full Decision Record (B1–B7, maintainer-resolved) and gate record: docs/superpowers/plans/2026-07-13-wallet-sdk-accounts-slice.md.

Stacked on sdk/auth-slice (#1166). After #1166 merges: rebase onto master, retarget, re-verify (two-green-PRs rule).

What lands

Session key plumbing (session-keys.ts): memoized, generation-fenced getters over Open Secret (encryption keypair, cashu seed, spark mnemonic, locking xpub, spark identity pubkey), cleared in onSessionEnded — a different user can never be served the previous user's keys.
sdk.accounts (get/list/cashu.add) over one shared domain→projection mapper; AddCashuAccountParams pinned ({ name, mintUrl, currency, purpose }); AgicashSdk Pick grows 'accounts'.
B1 (maintainer ruling): projection-typed, runtime-fat cache. The web ['accounts'] cache holds projection-typed objects fetched via sdk.accounts.list(); hidden domain fields ride along at runtime until step 18 (type-level strip now, physical then). The shared mapper is the only cache entry point (queryFn, realtime, add onSuccess, ensure seed, claim upserts). Reality-class: sdk.accounts.* is TYPE-honest / RUNTIME-fat-until-18, intended and time-boxed; assumes web-internal consumers only during the window.
/temporary bridge v2: internal-repo accessor + toDomainAccount() checked cast (throws MissingDomainFieldsError naming missing fields — never a bare cast) + mapper re-export. All carry step-18 removal notes.
sdk.user.ensure(): the _protected.tsx bootstrap ported verbatim (key derivation, default accounts, Zod-aware retry, session-fenced memo); returns { user, accounts } projection-typed. Timestamp params are replayed acceptance times from pending-terms storage (maintainer-confirmed intent, JSDoc'd). Web glue keeps master's structure exactly: short-circuit, prefetch warm-up, conditional seeding.
Web flip (48 files): root Account/CashuAccount/SparkAccount/ExtendedAccount are now the projections (domain types import from /temporary); display reads .balance off the cache; money flows stay domain end-to-end through sanctioned unwrap seams.

Unwrap seams (every hidden-field read passes `toDomainAccount()`)

account-hooks: the four getter hooks unwrap internally; useDefaultAccount/useAccountOrDefault return domain.
buy-input / send-input / receive-input: unwrap at the selector option build; account-selector renders domain via getAccountBalance (master verbatim).
send-provider getAccounts; routes _protected.buy.checkout / receive.cashu / receive.spark / transfer.$destinationAccountId / send; claim route.
account-proofs, transaction-additional-details, use-track-spark-account-balances (spark listener) at their seam.
receive-cashu-token-hooks: unwraps for getSourceAndDestinationAccounts; maps the domain source account back through the shared mapper for display items.

Declared (questions and transitional items, not silent changes)

accounts.get(id) is not session-gated (master's repository posture — RLS scopes it); list/add gate on the session for userId, user.* gates every verb. Parity kept; consistency call open to review.
withRetry/delay ported into the SDK lib; web copies remain for unmigrated consumers (dies with their slices).
account-repository-hooks relocated to features/receive/ (not deleted): its only consumers are unmigrated receive repos constructing AccountRepository synchronously; reshaping them is step 8–16 scope. account-service-hooks deleted.
One parity FLAG: session-keys.ts ports master's hardcoded MAINNET for the spark identity pubkey (master's own TODO).
sparkDebugLog stays a named /temporary exception (maintainer ruling); dies at 18 with its call site.

Verification

fix:all clean · workspace typecheck green (all 9 packages incl. web) · tests exit 0: wallet-sdk 96 (25 new: projection completeness type+runtime, mapper single-entry, checked-cast thin-object throw, ensure memo/retry, key fencing across session end), web 36, money 14, bolt11 9, ecies 18, cashu 35. Chain re-run independently at the keeper gate on the frozen tree.
Browser smoke (build box): boots clean, landing/signup render with zero console errors, guest flow drives to ToS and initiates registration; blocked there by the dev enclave's server-side 403 for this box's origin (environmental — master fails identically here). Post-auth surfaces want a maintainer-env smoke; the ensure/mapper/fencing paths are unit-covered.

🤖 Generated with Claude Code

vercel · 2026-07-13T09:46:39Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
agicash	Ready	Preview, Comment	Jul 19, 2026 6:10pm

supabase · 2026-07-13T09:46:46Z

This pull request has been ignored for the connected project hrebgkfhjpkbxpztqqke because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.

Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

orveth · 2026-07-13T18:41:19Z

Automated cross-model review — accounts slice (step 6)

Scope: step-6 slice diff 937e23a9..1dba0430 (64 files: SDK accounts namespace + key plumbing + 48-file web import sweep). Method: four independent lanes — Claude (Opus) general + Claude (Opus) adversarial + GPT‑5.5 (codex) general + GPT‑5.5 (codex) master-parity — plus orchestrator verification: every finding below was re-derived against the code (and master / the frozen tree) before inclusion. Additive to the maintainer's own review.

Verdict: well-built, faithful, well-tested slice. The projection-vs-domain split is applied consistently at every flipped call site, the fat-cache integrity design holds under adversarial attack (the toDomainAccount() checked cast genuinely throws on a thin object; the shared mapper is the only cache-entry point; key getters are generation-fenced against cross-user leaks; balance math is unchanged), the 48-file web sweep is coherent with no dangling /temporary imports, and the root-type-shadow deletion is sound. One MED, five LOW. No HIGH. The test suite is independently green on the frozen tree (re-run, not gate-trusted: bun install / typecheck / 96 SDK / 36 web, all exit 0, uncapped).

MED — `sdk.user.ensure()`'s result-memo can replay a stale account snapshot into the `['accounts']` cache (undeclared parity regression)

packages/wallet-sdk/domain/user/user-api.ts:148-149,200,207 + apps/web-wallet/app/routes/_protected.tsx:55-70

ensure() memoizes { user, accounts } per session (ensureMemo, set at :200, returned on hit at :149) and is only invalidated on session end (reset() at :207) — never by a later account add / balance change. The _protected seed fires on !user || hasUserChanged, where user is the TanStack ['user'] cache; the memo-hit keys on the SDK's ensureMemo. These are different layers and can desync: if TanStack ['user'] is lost while the SDK session (and its memo) survive for the same user, the seed's !user branch calls ensure(), gets the stale memoized snapshot, and writes it into ['accounts'] — which has staleTime: Infinity, so it stays stale.

Consequence: newly-added accounts / updated balances vanish from the UI until some mutation or invalidation touches ['accounts']. Money-display staleness (not a fund-safety bug — spend paths unwrap to domain/DB truth).
Why it's a regression: master seeded ['accounts'] only from a fresh writeUserRepository.upsert, so its re-seed was always current DB truth. The SDK-level memo introduces staleness master didn't have. Not in the plan's Accepted-deltas list → undeclared.
Same root cause also drops terms params: on a memo-hit, ensure({ termsAcceptedAt }) returns the cached snapshot without persisting the new terms (latent today — the web passes terms once; JSDoc advertises the short-circuit without noting params are dropped).
Reachability is narrow — needs TanStack ['user'] eviction while the SDK session/memo survive (the exact trigger, e.g. gcTime GC vs a partial cache clear, isn't pinned; a full reload resets both and is safe). But staleTime: Infinity means once stale it stays stale.
Fix (any one): drop ensure()'s result-level memo and keep the short-circuit in _protected; or return a fromMemo/didUpsert signal and seed accounts only on a fresh upsert; or version-merge sdk.accounts.list() when seeding instead of a wholesale setQueryData replace (this also closes the unconditional-seed concern).

LOW findings

accounts.get(id) skips the session guard (domain/accounts/accounts-api.ts:52) — list() and add() both requireUserId(); get does not, so a logged-out call fails in key derivation rather than with a clean NoSessionError. RLS (session-token client) masks any cross-user read and this matches master's trust boundary, so no security impact — an API-consistency + error-quality gap. Fix: add requireUserId() to get (fail-fast + match siblings), or document it as intentionally session-agnostic.
config.spark.network accepted but not threaded / MAINNET hardcode (session-keys.ts:112-115) — verified faithful master-parity (master hardcoded 'MAINNET' and derived the identity via network.toLowerCase() → identical key); the code carries an explicit FLAG comment documenting the deliberate port. No wrong-network risk. Informational: wire config.spark.network when non-mainnet support lands, or the comment suffices as the deferral record.
toDomainAccount() guards key-presence, not value-definedness (domain/accounts/account-projection.ts:61-63) — field in account lets { wallet: undefined } past the loud-throw guard. Not reachable today (every sanctioned entry sets real values). Hardening: check account[field] == null.
Dead defaultAccounts export left behind (apps/web-wallet/app/features/user/user-hooks.tsx:97) — the list was copied into the SDK (user-api.ts:24) and its _protected consumer removed, orphaning the web original (zero remaining consumers; the file isn't otherwise in the diff). Delete it (and its now-unused isDevelopmentMode).
Non-transitive sort comparator (domain/user/user-service.ts:38) — getExtendedAccounts uses (_, b) => (b.isDefault ? 1 : -1), which floats defaults but scrambles the relative order of non-defaults. Harmless for current callers (they re-sort / .find) but now a public SDK export. Pre-existing logic relocated verbatim. Fix: (a, b) => Number(b.isDefault) - Number(a.isDefault).

Verified clean (multi-lane, hand-traced)

Shared mapper single-entry (every ['accounts'] write routes through it) · toDomainAccount genuine throwing assertion, tested with thin objects · session-key generation-fencing (in-flight-at-reset interleaving tested — no stale key served to a second user) · balance math (empty proofs → zero, Money.add, no raw arithmetic) · SDK bridge wiring + getInternalAccountRepository lifecycle · with-retry/delay · index.ts/temporary.ts export split · the receive-cashu-token and claim money paths (projection→domain→projection round-trip) · all pure import-swap .tsx. Test gaps noted (not findings): no ensure-with-changed-params test, no get-without-session test.

Lanes: opus-general · opus-adversarial · codex-deep-general · codex-deep-parity, orchestrated by keeper:sdk-auth. Reviews ran against isolated worktrees at 1dba0430; codex chunked 64 files into 2 parts per run (cross-chunk invariants swept by hand).

orveth · 2026-07-13T18:50:40Z

MED addressed in 0eda60b — sdk.user.ensure()'s result-memo is removed entirely: the upsert runs on every call with the given params, and the web's own getUserFromCache + hasUserChanged short-circuit (unchanged, master-verbatim) is the only gating layer — master's exact architecture. This closes both symptoms (stale-snapshot seed under TanStack user-cache eviction; terms params dropped on a memo hit) and makes the seed's freshness by-construction rather than mount-ordering-dependent. createUserApi returns UserApi directly again; the onSessionEnded reset wiring is gone with the memo. Tests: the three memo tests are replaced by always-upserts + params-carried-every-call regression tests (wallet-sdk 95 pass / 2 todo; web 36; full chain re-run green at the keeper gate).

The five LOW items are held for the maintainer's disposition, per the review's framing — none are folded into this fix commit.

orveth · 2026-07-13T18:53:13Z

Re-review — MED fix (`0eda60b5`)

Targeted re-review of the fix diff 1dba0430..0eda60b5 (4 SDK files, +44/−81, web byte-identical). Verified against the diff, not the gate.

The MED is closed. The fix is a pure removal that reverts to master's architecture — ensure()'s result-memo is deleted, so the host's own short-circuit is the only caching layer:

Memo structurally gone — ensureMemo, the memo-hit early-return, the = result assignment, the hasUserChanged helper (its only consumer), and createUserApi's reset are all removed; the factory now returns UserApi directly. No state remains that can go stale, so the stale-seed class is eliminated at the root rather than patched.
ensure() is now always-fresh and honors params every call — killing both MED symptoms (the stale-account seed and the dropped terms params on a memo-hit) in one stroke.
Session-end fencing intact — onSessionEnded drops only the now-vestigial resetUserSession(); clearSparkWallets() / clearAgicashMintAuthToken() / keys.reset() are untouched (no cross-user-key regression).
Web byte-identical — the _protected short-circuit is unchanged, so call frequency matches master (no redundant-ensure perf delta) and the seed is now always a fresh upsert.
JSDoc corrected — from "Memoized per session… short-circuit" to "Idempotent… the host owns caching."
Regression guards added — one test asserts the second ensure() returns a fresh result (no memo replay), one asserts terms params reach the upsert on every call. Both MED symptoms now covered.

Suite: author gate CHAIN_EXIT=0 (wallet-sdk 95 pass/2 todo, web 36); not independently re-run this round — the fix is a strict removal + added tests, and the base tree's suite was independently verified green in the prior round.

The five LOW findings from the first pass are untouched here (correctly held for maintainer disposition, not folded into this commit). Clean fix.

Re-review by keeper:sdk-auth.

pmilic021 · 2026-07-14T12:34:39Z

+      ] = await withRetry({
+        fn: () =>
+          Promise.all([
+            deps.keys.getEncryptionPublicKey(),


What is the cost of having this duplicated? I assume we are duplicating just for this interim phase until p18, and it wasn't deduped now for simplicity sake?

It was simplicity — and your question tipped the balance the other way: deduped in 34479d8. ensure() now consumes the accounts namespace's repository through a getAccountRepository dep, so the instance has exactly one construction path (it also subsumes the createRepository test seam — the mapping test injects the same dep). Your derivation-retry now wraps the repository acquisition alongside the three key getters, so a transient failure retries all four async paths while the memoized getters keep the only-refetch-what-failed property.

What is duplicated?

Context — that exchange predates the domain-types rebuild: at the time, ensure() constructed its own AccountRepository while the accounts namespace constructed another for sdk.accounts.*. The resolution was to dedupe: the accounts namespace owns the single construction path and ensure() consumes the same one through the getAccountRepository dep — which is what this line is. Nothing is duplicated today.

orveth · 2026-07-14T19:27:12Z

Post-review addition (maintainer-requested): the web now sources its key queries from the SDK's session keys instead of deriving its own copies — e0ea372 (getInternalSessionKeys() on the /temporary bridge, mirroring the repo accessor: module-scoped, dispose-cleared, step-18 removal note) + b5fb64f (useEncryption becomes one ['encryption'] suspense query over the SDK's getEncryption(); the private-key/public-key queries and their hooks are deleted — verified reader-less; seedQueryOptions/sparkMnemonicQueryOptions keep their keys but delegate their queryFns; the middleware warms shrink to encryption + seed + mnemonic).

Net: the ~5 duplicate enclave derivations per session bootstrap drop to zero; one derivation authority; raw private-key bytes leave the query cache as a side effect.

One declared deviation from the discussed sketch (smaller, same direction): xpubQueryOptions is KEPT untouched — its getCashuCryptography.getXpub() consumer feeds the cashu receive-quote path, so only its middleware warm was dropped. Failure surfacing is preserved because ensure() derives the same locking xpub SDK-side in the middleware; the residual cost is one lazy xpub derivation at first receive-quote, retired when receive migrates (steps 9–12).

Verified at the keeper gate: full chain re-run on the frozen tree (fix:all clean, 9/9 packages typecheck, tests exit 0 — wallet-sdk 99, web 36).

orveth · 2026-07-17T12:55:06Z

On balance / domain-vs-projection returns (deferred here from jbojcic's #1166 review of sdk/accounts.ts — this PR is that contract's implementation, so the answer belongs here):

Why it currently is the way it is. A cashu account's balance is derived state — sumProofs over its proofs — so the domain type deliberately doesn't store it: a stored field would be a second source of truth that can drift from the proofs (master computes it at render via getAccountBalance for the same reason; spark differs because its balance comes from the wallet service, not from local data). The Omit<> projection implements the contract ruling in force when this slice was planned (#1164: wallet handles / proofs / keysetCounters don't cross the public surface), which this PR's decision record (B1) turned into the type-level-strip-until-step-18 arrangement — the shared mapper attaches balance at the single point where account objects enter the web cache, preserving the one-computation-point property.

What changes under the expose-full-domain-types direction (the direction #1166 has now taken for the receive/send equivalents): the projection rationale collapses, and accounts can align cleanly — declare balance on the domain CashuAccount and attach it in AccountRepository.toAccount, which is already the single constructor every account object flows through (list, realtime row mapping, add, ensure) — the anti-drift property survives intact, one layer deeper. sdk.accounts.* and ensure then return domain types directly, and this PR's projection machinery deletes wholesale: the projection types, the shared mapper, the toDomainAccount checked cast, the unwrap seams in the web hooks, and the type-honest/runtime-fat transitional record — a net-negative, contained diff (deletions dominate; the web's domain-type imports flip back to the root).

The call is yours + petar's, not mine — it reopens B1, which petar ruled explicitly for this slice. Two coherent end-states:

Align now, pre-merge on this branch: cheapest total cost if the full-domain-types direction is contract-wide — this PR becomes the first slice fully on it, and nobody migrates these files twice.
Merge as-ruled, align later with the spec-doc update: keeps the ruled design; costs a second pass over the same surface when accounts follows.

Sized and ready to execute either way on your word.

orveth · 2026-07-17T16:42:57Z

Rebased onto the domain-types contract (d5d2f18 landing in #1166: domain Account/CashuAccount/SparkAccount exposed directly, no balance field — the direction decided for the question deferred here). The base also restructured (sdk/ → domain/sdk/, 16 commits), so the branch was rebuilt as 8 clean commits rather than replaying 17 through conflicts; the pre-rebase state is preserved at sdk/accounts-slice-pre-domain.

What changed vs the previous head:

The projection machinery is gone (mapper, checked cast, unwrap seams, projection types) — the cache holds domain accounts, balance reads are getAccountBalance(account) exactly like master, and getExtendedAccounts/isDefaultAccount keep their clean domain signatures (the generic hack died with the projections).
AddCashuAccountParams pinned in the new contract file: { name, mintUrl, currency, purpose }.
Everything operational carried over intact: sdk.accounts (get/list/cashu.add over the instance's repository), sdk.user.ensure() (merged with the base's split write repositories + withRetry around derivation and upsert), session-key plumbing + fencing (reset rides the base's session-teardown clear list), both /temporary accessors, the web re-sourcing (list/get/add/realtime/ensure glue), withRetry/delay in @agicash/utils, and the key-query delegation round (xpubQueryOptions untouched — live getXpub money-path consumer).

Verified at the keeper gate on the rebuilt head: fix:all clean, all packages typecheck, tests exit 0 (wallet-sdk 105, web 36); repo-wide grep confirms zero projection references remain. Independent delta review running.

Add createSessionKeys: per-session memoized getters for the encryption keypair, cashu seed, spark mnemonic, cashu locking xpub, and spark identity public key, derived from Open Secret. Each memo is generation-fenced so a derivation started before reset() cannot repopulate the cache for the next session, and rejections are not cached so a retry can recover. Wire keys.reset() into the AgicashSdk onSessionEnded teardown alongside the existing session-token, spark-wallet, and mint-auth-token clears, so a signed-in user's key material never survives into the next login. The accounts and user namespaces consume these getters in later commits. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Implement createAccountsApi: get(id), list() (session-gated for the userId), and cashu.add(params) over an internal AccountRepository built from the db, session keys, and spark config. The namespace returns the domain account types directly per the accounts contract (#1166) — no projection mapping. cashu.add re-injects type:'cashu' and the session userId before the service call, and list()/add gate on the session while get() relies on RLS (repository posture). getRepository is exposed alongside the api so the /temporary bridge and the user namespace's ensure() build the repository through one path. Pin AddCashuAccountParams to { name, mintUrl, currency, purpose } and wire the accounts field into AgicashSdk, replacing the not-implemented getter. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Expose getInternalAccountRepository through '@agicash/wallet-sdk/temporary': a module-scoped accessor that hands unmigrated receive/send flows and realtime row mapping the live instance's internal domain accounts repository, built through the same path as sdk.accounts.*. It throws 'No live AgicashSdk instance' before create and after dispose (the module reference is cleared on dispose alongside liveInstance), so the domain repository never leaks onto the public AgicashSdk surface. Removed at step 18 when those flows read from the SDK. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

The SDK's user.ensure() needs the retry helper the web owned, so lift withRetry and delay from apps/web-wallet/app/lib into @agicash/utils (named exports, barrel re-exported) and delete the web copies. Flip the remaining consumers — the cashu receive-quote hook, the protected-route bootstrap, and the e2e Open Secret fixture — onto @agicash/utils, and add the workspace dependency to the e2e package. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Add user.ensure(params) to the contract and implement it on the user namespace: it derives the encryption public key, cashu locking xpub, and spark identity public key, upserts the user row (creating the default accounts and persisting the keys on first sign-in) through the base UpsertUserRepository, and returns { user, accounts } domain-typed for the host to seed its caches. The upsert returns domain accounts already, so ensure returns them as-is — no mapping. The key-derivation batch and the upsert each run under withRetry (matching the resilience the host's query layer gave master); the upsert retry is Zod-aware so a validation error fails fast. EnsureUserParams carries the replayed pending-terms acceptance timestamps, distinct from acceptTerms' in-session boolean stamps. ensure builds its repository through the accounts namespace's getRepository, so the whole instance shares one construction path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Flip the accounts data path onto the SDK namespace: accountsQueryOptions' queryFn calls sdk.accounts.list(), useAccountOrNull's lazy fetch calls sdk.accounts.get(id), useAddCashuAccount calls sdk.accounts.cashu.add (callers drop the now-implicit type: 'cashu'), and the realtime ACCOUNT_CREATED/UPDATED handlers map rows through getInternalAccountRepository().toAccount. The cache still holds domain accounts, so every consumer (getAccountBalance, wallet/proofs readers) is unchanged. account-service-hooks is deleted (the service lives behind sdk.accounts.cashu.add) and account-repository-hooks moves to features/receive, its only remaining consumers being the unmigrated cashu receive-quote and receive-swap repos. Extract getExtendedAccounts and isDefaultAccount from UserService into standalone pure functions exported from the package root, and flip their consumers (the accounts hooks, the claim route, and the SDK-internal claim service). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Route _protected's bootstrap through sdk.user.ensure(): it replaces the inline key-derivation + upsert, seeds the user and accounts caches from the public return, and keeps master's structure (cache short-circuit, session-token warm, ensureBreezWasm placement, conditional seed on the upsert branch). Source the web-side key queries from the live instance's session keys via a new getInternalSessionKeys /temporary accessor: useEncryption collapses to a single ['encryption'] query over getEncryption(), and the cashu-seed and spark-mnemonic query fns delegate to the same getters. The per-key encryption query options and their hooks are deleted (only encryption-hooks and the two route warms read them), sparkIdentityPublicKeyQueryOptions is dropped (its only reader was the ensure warm), and xpubQueryOptions is untouched. The bootstrap warms encryption, seed, and mnemonic concurrently with ensure() so the unmigrated receive/send/claim repos keep master's warm-cache and fail-in-the-middleware behavior. The web defaultAccounts copy is removed now that ensure() owns it SDK-side. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Bring the step-6 decision record onto the branch and append a supersession entry: the maintainer's contract-level domain-types ruling (d5d2f18, #1166) superseded B1's projection apparatus after the slice was first built, so sdk.accounts.* returns domain types directly (no mapper, no checked cast), B5 balance reads revert to getAccountBalance, and the runtime-fat reality-class record and the step-18 physical strip retire as moot. The record's prior entries stand as the decision trail. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

orveth · 2026-07-19T17:51:32Z

Rebased onto master following the #1166 merge and retargeted the PR (base: sdk/auth-slice → master).

Clean replay of the same 8 commits: d9a1fa26 → 0a34e6c9 (no conflicts, no content changes)
Verification re-run on the rebased tree: lint + typecheck + tests green (wallet-sdk 105/105, web-wallet 36/36)

Next: sweeping all #1166 review comments for reasoning applicable to this slice, per @petar; will report findings here.

… scope Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

orveth · 2026-07-19T18:10:15Z

#1166 review-comment sweep — applicability to this PR

Per @petar's ask: enumerated all #1166 commentary (55/55 review threads, 192 inline comments, 3 issue comments — pagination exhausted, counts cross-checked against the REST comment total) and audited this PR's full diff against every ruling that generalizes beyond the auth slice.

Applied (2 new commits)

91282973 — AbortController fence for the session key memos. session-keys.ts used a hand-rolled generation counter to keep an in-flight derivation from populating the cache after reset() — the same pattern #1166 replaced in auth-service with an AbortController session scope ("scope.aborted answers 'what is this for' at the call site"). Now mirrors auth-service: the keys hold a scope controller, reset() aborts and replaces it, an in-flight derivation captures the signal at start and neither caches nor releases the in-flight slot once aborted. Behavior identical; all existing tests (including the in-flight-derivation-at-reset test) pass unmodified.

db746ef8 — comment cleanup per review conventions. Comments only, no code: removed prior-code/branch narration ("master derived these…", "preserves master's…", "ported as-is"), kept the load-bearing facts (retry re-derives only failed keys; the _protected warms feed unmigrated repositories; network fixed to mainnet, not yet wired to config.spark.network); "the web" → "the host" in comments this branch added; dropped grammar-metaphor wording.

Verification on the pushed head: lint + typecheck clean, wallet-sdk 105/105, web-wallet 36/36.

Already satisfied (verified against the diff, no change needed)

Domain types direct, no projections — zero projection/balance-field remnants tree-wide; consumers sum proofs via getAccountBalance.
domain/sdk/ taxonomy + full-Sdk implementation — accounts fills the throwing NotImplementedError getter; no Pick-widening.
setDefaultAccount(userId, account) full-entity contract — untouched; isDefaultAccount/getExtendedAccounts operate on the passed entity, no id-plus-refetch.
Repository split seam — ensure() consumes the base's UpsertUserRepository(db, accountRepository) with one repository construction path shared by the namespace, the /temporary accessor, and ensure; the web no longer constructs it.
In-band results, not events — ensure() returns { user, accounts } and the host seeds caches from the return value.
Primitives over call-site patches — withRetry/delay promoted to @agicash/utils; no call-site papering.

Held as questions (parity vs. ruling — your call, no code changed)

storageDir ?? './.spark-data' at the SDK composition root (sdk.ts:135). The value is master parity and the config field is documented optional-with-default; the feat(wallet-sdk): auth & user slice (step 5) #1166 generateToken ruling ("why even have the default?") could be read to require the host to pass it. Fine as is, or make it required?
Mainnet hardcode in session-keys.ts ignores the available config.spark.network (carried master parity, flagged in-code). Port-as-is stands, or wire it now?
DisposedError posture: user/accounts verbs don't throw DisposedError after dispose() (they're session-gated only) — consistent with the base's own user namespace, so flagging as a shared posture question, not a divergence. Relatedly, the /temporary accessors throw a raw Error rather than a typed SdkError.

jbojcic1 · 2026-07-20T09:01:06Z

Why is this file on the top level and not in one of the folders?

It's shared plumbing with no single owner: the user namespace (ensure() derives the public keys), the accounts namespace (the repository takes the seed/mnemonic getters), and the SDK root (construction, reset on session end) all consume it, so any domain folder would force cross-domain imports. That said, it's the only non-config file at the top level — it can move to a shared folder (e.g. lib/) in the next code round if you prefer.

jbojcic1 · 2026-07-20T09:06:57Z

      userService.setDefaultAccount(requireUserId(), params.account, {
        setDefaultCurrency: params.setDefaultCurrency,
      }),
+    ensure: async (params) => {


This naming is weird. Ensure what?

The full call reads sdk.user.ensure(...) — meant as "ensure the user" — but agreed it's ambiguous, especially at the implementation site. What it ensures: the signed-in auth user has a wallet user row and default accounts — creates them on first sign-in, updates when auth data changed, no-ops otherwise. Renaming per the other thread.

jbojcic1 · 2026-07-20T09:08:44Z

+            giftCardMintTermsAcceptedAt: params.giftCardMintTermsAcceptedAt,
+          }),
+        retry: (attemptIndex, error) => {
+          if (error instanceof core.$ZodError) {


What is this check for? Did we have that before?

Yes — ported from master: the same predicate lives in master's _protected.tsx retry (line 143 there). A Zod validation failure means the upsert response didn't match the expected schema — that's deterministic, retrying can't change it, so we bail; anything else retries up to two more attempts.

jbojcic1 · 2026-07-20T09:09:17Z

+      ] = await withRetry({
+        fn: () =>
+          Promise.all([
+            deps.keys.getEncryptionPublicKey(),


What is duplicated?

jbojcic1 · 2026-07-20T09:09:58Z

+/**
+ * Returns true if the account is the user's default account for its currency.
+ */
+export function isDefaultAccount(


Why these changes?

Two changes here: (1) isDefaultAccount/getExtendedAccounts moved from statics on UserService to standalone exported functions — the SDK contract exposes these two helpers to the host while the UserService class stays internal, and exporting the class just to reach two statics would put the whole service on the public surface. (2) The user parameter narrowed from User to a Pick of the two default-account ids, because that's all these functions read — callers don't need a full User to use them. Behavior is unchanged from the master statics.

jbojcic1 · 2026-07-20T09:53:11Z

+    private_key_derivation_path: encryptionKeyDerivationPath,
+  }).then((response) => response.public_key);
+
+export function createSessionKeys(deps: SessionKeysDeps = {}): SessionKeys {


Why do we need to cache things here? Should this be cached in the web app (clients)?

It's per-session dedupe rather than app-level caching: the SDK's own internals hit these getters repeatedly — ensure() derives three public keys, and every repository construction takes the seed/mnemonic getters — and each underlying call is a round trip to the Open Secret enclave. The memo makes each derivation happen once per session; reset() drops it at session end. The web still caches for its own consumers in the query layer, but those queryFns delegate to these getters, so the enclave work happens once either way. Before this PR the web's five bootstrap key queries each derived independently — same keys, five enclave round trips.

jbojcic1 · 2026-07-20T09:53:54Z

+// repository on the public AgicashSdk surface.
+let liveAccountRepository: (() => Promise<AccountRepository>) | undefined;
+
+// The live instance's session keys, reached only through the


What does live mean here? Do we have something else that is not live?

"Live" = the instance currently constructed and not yet disposed. There's no second kind of instance — the word distinguishes the current one from a disposed (or replaced) one across the construct→dispose lifecycle: the module-scoped slot is set on construction, cleared on dispose, and the accessors throw when nothing is live. Happy to rename to current/active if that reads better.

jbojcic1 · 2026-07-20T10:19:27Z

+    // the same derivation ensure() runs SDK-side. Removing a warm before its
+    // domain migrates into the SDK breaks that domain's bootstrap.
+    const [{ user: upsertedUser, accounts }] = await Promise.all([
+      sdk.user.ensure({


This seems odd now. The code checks if the data on the supabase and OS user is different and if yes calls ensure which doesn't even take that data in

That's master's own short-circuit, kept as-is (master _protected.tsx:90: if (!user || hasUserChanged(user, authUser)) gating the inline upsert). It only decides WHETHER to call — skip the bootstrap when the cached wallet user already matches the auth user. ensure() doesn't need the compared data passed in because it reads the auth user from the SDK's own session and derives everything else internally; the web comparison supplies no inputs, it just avoids an unnecessary round trip on remounts.

jbojcic1 · 2026-07-20T10:19:41Z

-      queryClient.ensureQueryData(
-        sparkIdentityPublicKeyQueryOptions({ queryClient, network: 'MAINNET' }),
-      ),
+    // These warms populate the cache entries that the receive/send/claim


Warms? I have no idea what this comment is saying

Fair hit — "warms" is prefetch jargon; will reword to "prefetches" in the next code round. What it's saying: these ensureQueryData calls prefetch the cache entries (encryption / seed / mnemonic) that receive, send, and claim code — not yet migrated into the SDK — still reads at bootstrap. Removing one before its feature migrates breaks that feature's startup.

jbojcic1 · 2026-07-20T10:20:30Z

+        termsAcceptedAt,
+        giftCardMintTermsAcceptedAt,
+      }),
+      queryClient.ensureQueryData(encryptionQueryOptions()),


So now some stuff is cached here some in the sdk? Seems like encryption keys are cached on both places so not sure I follow what is going on

Close, but the split is deliberate: raw key material is memoized only in the SDK (once per session, dropped at session end). The web query layer caches the consumer-facing objects built FROM it — ['encryption'] holds the Encryption helper, not private-key bytes — and its queryFns delegate to the SDK getters, so the derivation itself happens once, SDK-side. Encryption keys are no longer cached in two places: before this PR the web cache held the raw private-key bytes; that entry is deleted. That was the point of the dedupe round — one derivation authority, raw bytes out of the query cache.

vercel Bot deployed to Preview July 13, 2026 09:47 View deployment

vercel Bot deployed to Preview July 13, 2026 09:52 View deployment

vercel Bot deployed to Preview July 13, 2026 14:06 View deployment

vercel Bot deployed to Preview July 13, 2026 14:09 View deployment

vercel Bot deployed to Preview July 13, 2026 14:12 View deployment

vercel Bot deployed to Preview July 13, 2026 15:34 View deployment

vercel Bot deployed to Preview July 13, 2026 16:22 View deployment

vercel Bot deployed to Preview July 13, 2026 18:10 View deployment

orveth changed the title ~~Sdk/accounts slice~~ feat(wallet-sdk): accounts slice (step 6) Jul 13, 2026

vercel Bot deployed to Preview July 13, 2026 18:51 View deployment

vercel Bot deployed to Preview July 14, 2026 12:17 View deployment

pmilic021 commented Jul 14, 2026

View reviewed changes

vercel Bot deployed to Preview July 14, 2026 14:30 View deployment

vercel Bot deployed to Preview July 14, 2026 19:28 View deployment

orveth force-pushed the sdk/accounts-slice branch from b5fb64f to 4bef09b Compare July 17, 2026 16:42

vercel Bot deployed to Preview July 17, 2026 16:43 View deployment

orveth force-pushed the sdk/accounts-slice branch from 4bef09b to d9a1fa2 Compare July 17, 2026 16:44

vercel Bot deployed to Preview July 17, 2026 16:45 View deployment

orveth force-pushed the sdk/auth-slice branch from 12fe387 to 48ea923 Compare July 17, 2026 17:33

orveth and others added 3 commits July 19, 2026 10:49

orveth and others added 5 commits July 19, 2026 10:49

orveth changed the base branch from sdk/auth-slice to master July 19, 2026 17:50

orveth force-pushed the sdk/accounts-slice branch from d9a1fa2 to 0a34e6c Compare July 19, 2026 17:50

vercel Bot deployed to Preview July 19, 2026 17:51 View deployment

orveth and others added 2 commits July 19, 2026 11:05

refactor(wallet-sdk): fence session key memos with an AbortController…

9128297

… scope Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

docs: comment cleanup per maintainer review conventions

db746ef

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

vercel Bot deployed to Preview July 19, 2026 18:10 View deployment

jbojcic1 reviewed Jul 20, 2026

View reviewed changes

Uh oh!

Conversation

pmilic021 commented Jul 13, 2026 • edited by orveth Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What lands

Unwrap seams (every hidden-field read passes toDomainAccount())

Declared (questions and transitional items, not silent changes)

Verification

Uh oh!

vercel Bot commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

supabase Bot commented Jul 13, 2026

Uh oh!

orveth commented Jul 13, 2026

Automated cross-model review — accounts slice (step 6)

MED — sdk.user.ensure()'s result-memo can replay a stale account snapshot into the ['accounts'] cache (undeclared parity regression)

LOW findings

Verified clean (multi-lane, hand-traced)

Uh oh!

orveth commented Jul 13, 2026

Uh oh!

orveth commented Jul 13, 2026

Re-review — MED fix (0eda60b5)

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orveth commented Jul 14, 2026

Uh oh!

orveth commented Jul 17, 2026

Uh oh!

orveth commented Jul 17, 2026

Uh oh!

orveth commented Jul 19, 2026

Uh oh!

orveth commented Jul 19, 2026

#1166 review-comment sweep — applicability to this PR

Applied (2 new commits)

Already satisfied (verified against the diff, no change needed)

Held as questions (parity vs. ruling — your call, no code changed)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbojcic1 Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbojcic1 Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

pmilic021 commented Jul 13, 2026 •

edited by orveth

Loading

Unwrap seams (every hidden-field read passes `toDomainAccount()`)

vercel Bot commented Jul 13, 2026 •

edited

Loading

MED — `sdk.user.ensure()`'s result-memo can replay a stale account snapshot into the `['accounts']` cache (undeclared parity regression)

Re-review — MED fix (`0eda60b5`)

jbojcic1 Jul 20, 2026 •

edited

Loading

jbojcic1 Jul 20, 2026 •

edited

Loading